cpp - Using the return value of a strcpy or related string copy function in an if statement by raulgarciamsft · Pull Request #695 · github/codeql

raulgarciamsft · 2018-12-14T23:45:52Z

Equivalent to http://go.microsoft.com/fwlink/?LinkID=244784

…ion in an if statement

jbj

Thank you for contributing this query. I tested it on 49 projects, and it gave no results. That's a good sign since hopefully the errors found with this query are very rare.

jbj · 2018-12-17T11:04:04Z

+
+from IfStmt ifs,  
+FunctionCall func
+where isStringComparisonFunction( func.getTarget().getQualifiedName() )


In C++, when imported via #include <cstring>, these functions are named std::strcpy etc. If you use getName instead of getQualifiedName, you'll match those as well. That might also match unrelated functions like foo::bar::strcpy, but hopefully nobody names a function strcpy without ensuring it has the same behaviour as the one from the standard library.

It is a simple change, and hopefully it will not cause false positives, as you mentioned, it is very unlikely that somebody would name a function strcpy without it behaving similarly to the standard library. Changing it

jbj · 2018-12-17T11:08:55Z

+  or functionName = "_mbsncpy_l"
+}
+
+from IfStmt ifs,  


Why limit this to IfStmt and to particular operations in its condition? Shouldn't the warning apply at any point where the result of a string copy function is converted to a Boolean type? That would be like in #211.

I added conversions to Boolean values, and also adding a dataflow to the result being used in logical/equality operations. Running a few more tests to make sure I am not missing anything.

I tested the query again: https://lgtm.com/query/4616339080794271386/

There are results in Microsoft CoreCLR that I presume to be false positives. I suggest using only intraprocedural data flow (DataFlow::localFlow) to avoid results like that.

Fixed. I run the same tests again with the new prototype, and I found no false positives. Thanks

jbj · 2018-12-17T12:18:56Z

+      and func = eop.getAChild()
+    )
+  )
+select func, "Incorrect use of function " + func.getTarget().getQualifiedName() + ". Verify the logic and replace with a secure string copy function, or a string comparison function."


We generally don't give advice in alert messages on how to remedy the problem. It's okay for an alert to be terse. I suggest

"Return value of " + func.getTarget().getQualifiedName() + " used in condition"

Sounds good. I will change the string to only indicate that the return value from these functions does not reserve any value to indicate an error.

NOTE: In C++, this new approach is showing some cases twice (boolean conversion && used in a logical condition), but otherwise I miss some scenarios.
In C, it works well as there is no implicit boolean type conversion when used in logical conditions

jbj · 2018-12-17T12:20:20Z

+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id cpp/string-copy-function-in-if-condition


Depending on how we address some of my other remarks, it may be appropriate to change the id, name, description, and file name.

jbj · 2018-12-17T12:26:04Z

+    {
+    }
+
+    if (SomeFunction() && strcpy(szbuf1, "test")) // Bug, binary logical operator


Please add a test case that's one more level deep. For example, add a ! in front of strcpy here.

I think this is the last of my comments that still isn't addressed. I suggest that you change isStringCopyUsedInCondition so it doesn't require that the logical operations are contained in a condition. Don't we also want to flag code like status = !strcpy(s, "foo");?

I completely forgot about this comment during the break. Sorry about that.

raulgarciamsft · 2018-12-17T21:56:11Z

It seems like the QHelp-Preview check failed, but I have no visibility into the failure. Please let me know what I should change.

…t/ql into users/raulga/c6324

raulgarciamsft · 2019-01-03T18:15:43Z

C++ QL Tests failed (nuget failure during QL install step). I am not sure if there is anything I can do to fix it on my side.

jbj · 2019-01-03T19:55:49Z

Thanks. I've triggered our Jenkins-based tests instead of the broken ones, and I'll follow up tomorrow when they've completed.

jbj · 2019-01-04T13:21:05Z

To get rid of the duplicates, you can pick one of the messages to have precedence and suppress the other one when it's there. Change

  isStringCopyCastedAsBoolean(func, expr1, msg)
  or isStringCopyUsedInCondition(func, expr1, msg)

to

  isStringCopyCastedAsBoolean(func, expr1, msg) and
  not isStringCopyUsedInCondition(func, expr1, _)
  or
  isStringCopyUsedInCondition(func, expr1, msg)

I changed to detect any logical operation usage (i.e. !, ==), but I kept usage in a conditional directly as a separate detection condition. I found no false positives on the projects you shared with me previously.

jbj

The tests have passed, so I'll merge this.

cpp - Using the return value of a strcpy or related string copy funct…

16f2bac

…ion in an if statement

raulgarciamsft requested a review from a team as a code owner December 14, 2018 23:45

raulgarciamsft added 2 commits December 14, 2018 15:46

Merge branch 'master' into users/raulga/c6324

f8ab945

Update .gitignore

0531602

jbj reviewed Dec 17, 2018

View reviewed changes

rdmarsh2 reviewed Dec 17, 2018

View reviewed changes

Comment thread cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.qhelp Outdated

jbj added the C++ label Dec 18, 2018

raulgarciamsft and others added 4 commits January 2, 2019 16:23

Fixing the code based on PR feedback.

28932e8

Update UsingStrcpyAsBoolean.cpp

e7bc3e6

Switched to DataFlow::localFlow to avoid false positives.

2c1d7bb

Merge branch 'users/raulga/c6324' of https://github.com/raulgarciamsf…

89c045b

…t/ql into users/raulga/c6324

raulgarciamsft added 2 commits January 4, 2019 10:45

Removing duplicated results

880306c

Fixing conditional only issue.

18bb669

I changed to detect any logical operation usage (i.e. !, ==), but I kept usage in a conditional directly as a separate detection condition. I found no false positives on the projects you shared with me previously.

jbj approved these changes Jan 10, 2019

View reviewed changes

jbj merged commit 9219214 into github:master Jan 10, 2019

s0 mentioned this pull request Mar 26, 2019

C false positive: Return value of strncpy used in a logical operation. #1153

Closed

raulgarciamsft deleted the users/raulga/c6324 branch October 4, 2019 16:58

Uh oh!

Conversation

raulgarciamsft commented Dec 14, 2018

Uh oh!

jbj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raulgarciamsft commented Dec 17, 2018

Uh oh!

Uh oh!

raulgarciamsft commented Jan 3, 2019

Uh oh!

jbj commented Jan 3, 2019

Uh oh!

jbj commented Jan 4, 2019

Uh oh!

jbj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants